fix: Clean up attributes for traces and metrics#12677
Conversation
blakeli0
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors observability attribute collection by removing artifact and version attributes, centralizing error attribute logic into a new method, and updating tracing logic to use this centralized method. The review identified a consistent typo in the new method name 'gettErrorAttributes', which should be corrected to 'getErrorAttributes' across the codebase, including in the corresponding test method names.
...m-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/GoldenSignalsMetricsTracer.java
Outdated
Show resolved
Hide resolved
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/ObservabilityUtils.java
Outdated
Show resolved
Hide resolved
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/SpanTracer.java
Outdated
Show resolved
Hide resolved
...tform-java/gax-java/gax/src/test/java/com/google/api/gax/tracing/ObservabilityUtilsTest.java
Outdated
Show resolved
Hide resolved
...m-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/GoldenSignalsMetricsTracer.java
Outdated
Show resolved
Hide resolved
| ObservabilityUtils.populateStatusAttributes(attributes, null, transport); | ||
| metricsRecorder.recordOperationLatency( | ||
| clientRequestTimer.elapsed(TimeUnit.NANOSECONDS) / 1_000_000_000.0, attributes); | ||
| recordMetric(null); |
There was a problem hiding this comment.
not a blocker for this PR, but I feel that recordMetric(null) gives the wrong impression of the logic (maybe it's just to me and I may be just reading it too fast). At a first glance, I read the method like record no metric. Only after reading the signature did it make sense that the first param correspond to the error.
Maybe ideally something like recordErrorMetric that takes in the error param and recordMetric that records the status? But I guess that requires a bit of duplication in the logic.
There was a problem hiding this comment.
Makes sense. I can definitely see the potential confusion. Let me give it more thoughts.
2 participants
This PR
exception.typeandgcp.client.repoattributes to metrics.gcp.client.artifactandgcp.client.versionfrom Span attributes since they are recorded as instrumentationName and instrumentationScope.ObservabilityUtils